Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core/common): Add @Search decorator to the available HTTP route handlers #10533

Merged
merged 4 commits into from
Apr 17, 2023

Conversation

Gustrb
Copy link
Contributor

@Gustrb Gustrb commented Nov 7, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #10420

What is the new behavior?

This PR adds the new Search method decorator to be a valid handler to routes.
This change was rather simple, since both express and fastify already support this little akward and rather new http method

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

coveralls commented Nov 7, 2022

Pull Request Test Coverage Report for Build 7ebb1617-a886-4ce7-8c0b-7e619c5abc22

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 93.778%

Totals Coverage Status
Change from base Build c2dbdd8a-105c-4233-97cc-e76da419e49f: 0.4%
Covered Lines: 6149
Relevant Lines: 6557

💛 - Coveralls

@Gustrb Gustrb changed the title Add search method feat(core/common): Add search method Nov 7, 2022
@Gustrb Gustrb changed the title feat(core/common): Add search method feat(core/common): Add @Search decorator to the available HTTP route handlers Nov 7, 2022
Comment on lines 83 to 87
public search(port: string | number, callback?: () => void);
public search(port: string | number, hostname: string, callback?: () => void);
public search(port: any, hostname?: any, callback?: any) {
return this.instance.search(port, hostname, callback);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this to right below public all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing!

@kamilmysliwiec kamilmysliwiec mentioned this pull request Nov 9, 2022
1 task
The search method is going to be anexed with the @Search decorator and
going to be used to reference a SEARCH request
This decorator uses the SEARCH method to construct a decorator
for a handler to a Search http request
@micalevisk
Copy link
Member

image

🤔

force-push it again @Gustrb

This update was really simple, because we dont need any kind of aliasing
due to fastify and express already supporting the search method for Application
@Gustrb
Copy link
Contributor Author

Gustrb commented Nov 9, 2022

Hmmm, did it and the issue seem to continue.

@micalevisk
Copy link
Member

nvm that's due to this: #10545 (comment)

@Gustrb
Copy link
Contributor Author

Gustrb commented Nov 9, 2022

oh, makes sense

@Gustrb
Copy link
Contributor Author

Gustrb commented Nov 10, 2022

btw, should I do something to fix it or should I wait the other issue?

@micalevisk
Copy link
Member

@Gustrb I guess you can apply the commit 9970ace here as well just to triggers the pipeline

@micalevisk
Copy link
Member

micalevisk commented Feb 6, 2023

I notice that this PR introduces a breaking change as the AbstractHttpAdapter is public and we're adding a new public mandatory method.


I feel that we could find a better way to add more route decorators... Express supports others routing methods that we won't bring to the core but we could have a way to let the consumer extending it if they want to.

An ideia: having a class with factory method that will use createMappingDecorator to create and register your route to a global storage.

@kamilmysliwiec
Copy link
Member

I feel that we could find a better way to add more route decorators... Express supports others routing methods that we don't bring to the core but we could have a way to let the consumer extending it if they want to.

There's a way - you can inject the HTTP adapter and register the route manually, instead of using a dedicated decorator.

@micalevisk
Copy link
Member

micalevisk commented Feb 6, 2023

yeah... I'm thinking on how we could allow the usage of such new http method at controller's class as well.

@kamilmysliwiec from client POV, does having yet another decorator factory to create any http route sounds bad to you?

something like this:

const Search = HttpRoute('SEARCH')

class FooController {
   @HttpRoute('GET')('/')
   getHello() {}

   @Search()
   bar() {}
}

then all the native ones would be just an alias to that. Seems overkill & too flexbile but I don't see any other way.

I could try to implement that while still supporting @nestjs/swagger plugin somehow, if you like the idea 😸

@micalevisk
Copy link
Member

I just notice that in the past Nest had the @RequestMapping({ method }) decorator factory. I don't know what were the reasons to remove that one but I guess we won't bring that abstraction to the core again.

@kamilmysliwiec kamilmysliwiec added this to the 10.0.0 milestone Apr 17, 2023
@kamilmysliwiec kamilmysliwiec changed the base branch from master to 10.0.0 April 17, 2023 11:14
@kamilmysliwiec kamilmysliwiec merged commit 7b05cd0 into nestjs:10.0.0 Apr 17, 2023
@wenerme
Copy link

wenerme commented Jun 16, 2023

I don't find any reference, I think this maybe the QUERY method. https://www.ietf.org/archive/id/draft-ietf-httpbis-safe-method-w-body-02.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants